-
Notifications
You must be signed in to change notification settings - Fork 840
API for Next Hop Strategy rebind during a transaction #12512
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
4009100
to
1d2557d
Compare
23edfb3
to
4805259
Compare
This adds 'c' friendly api calls to get and set strategies, to get the name of a given strategy and to get a strategy from the strategy factory by name. Also includes additions to header_rewrite, regex_remap and lua plugins.
a5939e5
to
8c1b741
Compare
Did you email the mailing list with the new API additions ? |
I think more tests need to be added for corner cases. Also on request am working on setting up a stress test case to ensure that converting the strategy shared_ptr to plain pointers doesn't result in faults. |
@jrushford Hey, do you want to take a look at this PR for us? |
@bryancall Yes, I've been talking with Bryan about it. I'll take a look but, give me a day as I'm currently traveling. |
+1 on more stress testing. I was worried that changing from shared_ptr to plain pointers might result in memory leaks or crashes when remap.config and strategies.yaml is reloaded. So far though it looks good to me. I'll spin up some VM's and test it. |
I stress tested using real time traffic replay (with strategies enabled), constantly forcing reloads every few seconds for several hours. I didn't observe any hiccups or issues. (A shame tsan is messy at the moment). Chris' presentation during the summit seemed to agree with my assessment that UrlRewrite which holds the StrategyFactory lives as long as any active transaction are still around, ensuring the strategies live as long as necessary. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed it and it looks fine to me and I discussed the stress testing with Brian and am satisfied that this is ok.
This adds to the ts api to allows the selected parent strategy to be changed during a transaction using a 'c' friendly API.
APIs added:
Set and get via strategy pointer.
Gets the name of the provided strategy
The below looks up a strategy by name by using the current transaction to get to the NextHopStrategyFactory.
Support added to:
Ideally there also should be a way to make the NextHopStrategyFactory pointer directly available to TSRemapNewInstance.